Merging this PR will degrade performance by 24.45%

❌ 9 regressed benchmarks
✅ 1097 untouched benchmarks
⏩ 1522 skipped benchmarks¹

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

_{Comparing aduffy/patched-array (5412a08) with develop (3c5fbd3)}

@claude review please

@claude i have addressed your main comments. please give another review.

@claude please review

@claude PR is ready for another review

Claude finished @a10y's task in 8m 28s —— View job

PR Review: PatchedArray basics and wiring

Tasks:

• Read PR context and understand the changes
• Read core array implementation files
• Read compute kernel implementations
• Read vtable implementation
• Read test/compat fixtures
• Provide comprehensive review feedback

Overall this is solid work — the design is clean, the test coverage is good, and the key operations (slice, filter, take, compare, scalar_at) are all well-structured. I found three correctness bugs of varying severity. Details below.

🔴 Bug 1: build() computes n_chunks incorrectly for sliced arrays

File: vortex-array/src/arrays/patched/vtable/mod.rs, line 246

let n_chunks = len.div_ceil(1024);  // WRONG for offset > 0

When a PatchedArray is sliced at a non-chunk boundary, array.offset > 0. For example, a slice from element 5 to element 1025 produces len = 1020, offset = 5, but the array spans 2 chunks. The lane_offsets buffer has n_chunks_actual * n_lanes + 1 = 2 * n_lanes + 1 entries, but build() reconstructs with n_chunks = 1020.div_ceil(1024) = 1, causing all downstream chunk/lane indexing to be wrong after a round-trip through serialization.

Fix: let n_chunks = (len + metadata.offset as usize).div_ceil(1024);

This matches how slice_chunks counts chunks and how SliceReduce selects chunk boundaries. All the invariants hold because n_chunks is always derivable from (len + offset).

Fix this →

🔴 Bug 2: Missing upper-bounds check in compare.rs's ApplyPatches::apply

File: vortex-array/src/arrays/patched/compute/compare.rs, lines 139–149

let bit_index = chunk * 1024 + patch_index as usize;
if bit_index < self.offset {
    continue;  // lower-bound guard ✓
}
let bit_index = bit_index - self.offset;
// No upper-bound check — can write past end of bits buffer!
if cmp(patch_value, self.constant) {
    self.bits.set(bit_index)
} else {
    self.bits.unset(bit_index)
}

The bits buffer has lhs.len bits. A patch in the final chunk at a position beyond offset + len (which can easily happen when len is not a multiple of 1024 — e.g., 2048-element array with a patch at index 2047 sliced to [0, 2040)) causes bit_index = 2047 - 0 = 2047 >= 2040, an out-of-bounds write.

Fix: Add if bit_index >= self.bits.len() { continue; } (or use self.offset + lhs_len as an explicit upper bound) after computing bit_index - self.offset. The equivalent guard already exists in apply_patches_primitive (line 369 of vtable/mod.rs).

Fix this →

🔴 Bug 3: Double-offset in slice_chunks when chunks.start == 0 and offset > 0

File: vortex-array/src/arrays/patched/array.rs, lines 138–168

let begin = (chunks.start * 1024).max(self.offset);  // = self.offset when start=0
let inner = self.inner.slice(begin..end)?;            // inner[0] = original[self.offset]
let offset = begin % 1024;                            // = self.offset again

When chunks.start == 0 and self.offset > 0 (i.e. filtering a non-chunk-boundary-sliced array that doesn't prune front chunks), the inner gets sliced by self.offset bytes AND the new offset field is still self.offset. During execute(), buffer.slice_typed(offset..offset+len) then skips another self.offset elements — effectively double-counting.

This path is reachable via filter.rs: when chunk_start == 0 && chunk_stop < n_chunks, slice_chunks(0..chunk_stop) is called. The existing test_filter_sliced test avoids this because it slices at chunk boundary 1024 (offset=0).

Fix: Distinguish the two cases:

• When chunks.start > 0: begin = chunks.start * 1024, new offset = 0 (chunk-aligned start)
• When chunks.start == 0: begin = 0, new offset = self.offset, new len = min(chunks.end * 1024, self.offset + self.len) - self.offset

Fix this →

🟡 Memory: slice_chunks retains all patch data

File: array.rs, lines 145–146

let indices = self.indices.clone();
let values = self.values.clone();

After slice_chunks, the indices and values arrays still hold patches for the entire original array, not just the retained chunk range. The lane_offsets correctly limits access, so this is not a correctness issue, but after a selective filter on a large array, all patches are still in memory. Worth a // TODO: trim indices and values to the patch range of lane_offsets[0]..lane_offsets[last] comment at least.

🟡 take_map builds the full patch hashmap even for selective takes

File: compute/take.rs, lines 92–107

for chunk in 0..n_chunks {
    for lane in 0..n_lanes {
        // ... inserts all patches into hashmap
    }
}

This scans all patches regardless of which elements are being taken. For a sparse take on a large array with many patches, it might be worth looping over the take indices first to identify which chunks are relevant, then only loading patches for those chunks. Not a blocker, but worth a // TODO for future optimization.

✅ Things that look good

• The transpose / scalar_at lane computation is consistent (both use absolute_index % n_lanes, where absolute_index is relative to the pre-offset chunk start, and n_lanes always divides 1024, so chunk boundaries don't break lane alignment).
• apply_patches_primitive correctly guards against both lower and upper out-of-bounds patches.
• The SliceReduce implementation is elegant — slicing always to chunk boundaries and tracking the sub-chunk offset is exactly right.
• The compat fixture is good, covering multi-chunk with patches in different chunks.
• Test coverage is thorough for the happy paths: rstest parameterization in slice.rs is especially nice.